야구게임 완성#1
야구게임 완성#1jslee-backend-engineer wants to merge 8 commits intojhta1805:masterfrom jslee-backend-engineer:master
Conversation
mintjordy
left a comment
mintjordy
left a comment
There was a problem hiding this comment.
이번주에는 메소드명, 변수명, 매직넘버 사용 위주로 피드백 남겼어요.
확인해주시고 수정해주세용~
고민되는 부분은 같이 고민 해봐요
src/main/java/Main.java
Outdated
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| BaseballService bs = new BaseballServiceImpl(); |
There was a problem hiding this comment.
축약된 변수명 😂 변수명은 의도가 분명히 드러나는 이름으로 사용하는게 좋아요.
클린코드 2장을 읽어보시면 좋을거 같아요 😀
아래 링크는 관련 포스팅인데 참고하세요.
https://namget.tistory.com/entry/%ED%81%B4%EB%A6%B0%EC%BD%94%EB%93%9C-2%EC%9E%A5-%EC%9D%98%EB%AF%B8-%EC%9E%88%EB%8A%94-%EC%9D%B4%EB%A6%84
| if (userNumber.length() != 3) { | ||
| System.out.println("세자리 수를 입력해주세요."); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
3이란 값처럼 코드에서 직접적으로 사용되는 값을 매직넘버라고 해요.
숫자 야구 게임에서 3이라는 값은 특별한 의미가 있는데요, 3이라는 값을 직접 사용하는 대신 상수로 만들어서 의미를 부여해보는 건 어떨까요?
힌트: 자바에서는 상수를 표현할때 보통 static final 을 이용해요. 해당 상수에 적절한 의미를 부여할 수 있도록 이름도 신경써서 지어볼까요?
| if (userNumber.equals("000")) { | ||
| return false; |
There was a problem hiding this comment.
마찬가지로 000이라는 값도 매직넘버가 될수 있겠죠?
src/main/java/BaseballService.java
Outdated
| public interface BaseballService { | ||
| String playGame(String userInput); | ||
| Boolean gameQuit(String userNumber); | ||
| Boolean isInt(String userNumber); | ||
| Boolean checkLength(String userNumber); | ||
| } |
There was a problem hiding this comment.
BaseballService를 인터페이스로 추출한 이유가 있을까요? 개인적으로 궁금해서 이유를 들어보고 싶네요 🙃
There was a problem hiding this comment.
일단은 나중에 baseball 게임 뿐만이 아니라 축구게임 등의 다른 게임으로 확장가능성을 생각했습니다. 생각해보니 baseballService 가 아니라 sportsGameService 가 맞을 거 같네요
| int bcnt = 0; | ||
| int scnt = 0; | ||
| int cn = Integer.parseInt(comNumber); | ||
| for (int i = 0; i < comNumber.length(); i++) { | ||
| String kcn = cn % 10 + ""; | ||
| int chk = reverseStr(userNumber).lastIndexOf(kcn); | ||
| if (chk > -1 && chk != i) { | ||
| bcnt++; | ||
| } | ||
| if (chk == i) { | ||
| scnt++; | ||
| } | ||
| cn = cn / 10; | ||
| } |
There was a problem hiding this comment.
축약된 변수명, 매직넘버들이 많이 보이는군요!
There was a problem hiding this comment.
checkNumber라는 메소드명으로는 해당 메소드가 무슨 기능을 수행하는지 파악하기 힘들어요.
결과적으로 이 메소드를 통해서 result 메소드의 결과가 반환되는데, 게임 결과를 반환하는 의미의 메서드 명이 더 적합하지 않을까 하는 개인적인 생각이 드네요~
| public String result(int bCnt, int sCnt) { | ||
| String res = ""; | ||
| if (bCnt > 0) { | ||
| res += bCnt + " ball "; | ||
| } | ||
| if (sCnt > 0) { | ||
| res += sCnt + " strike"; | ||
| } | ||
| if (bCnt == 0 && sCnt == 0) { | ||
| res = "nothing"; | ||
| } | ||
| return res; | ||
| } |
There was a problem hiding this comment.
해당 메소드는 바로 위 메소드에서만 사용하는 메소드인데 public 대신 privtate으로 설정하는건 어떨까요?
그리고~ 여기에서도 마찬가지로 축약된 변수명들과 매직넘버가 보이네요!
There was a problem hiding this comment.
해당 클래스에서 public 접근 제어자일 필요없는 메서드 모두 찾아서 접근제어자를 수정해봐요
메서드의 접근제어자는 될수 있으면 최대한 제약이 있는게 좋아요
1. 축약된 변수명 변경 bs -> baseballGame, bcnt -> ballCnt, scnt -> strikeCnt 2. 상수화 private final 로 변경 3. BaseballService 의 이름 변경 SportsGameService 로 이름 변경 이유 : 나중에 야구 말고 다른게임이 나왔을경우 확장하고 싶어서
1. static 추가
3 participants
1주차 미션 - 야구게임